- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
Add tidy check to deny merge commits #105058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tidy check to deny merge commits #105058
Conversation
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
61699c2    to
    117fd23      
    Compare
  
    | 
 @Nilstrieb do you have time to add this to the dev guide? | 
117fd23    to
    02ff160      
    Compare
  
    | And encountered again at #104195, so it will be good to have this lint. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The git handling looks good to me (or at least, good enough it shouldn't delay this longer), but I think I agree with Mark we should run this in CI. Can you either:
- change this to fail silently locally, but give a hard error if git fails in CI (see for how to detect it); or,Line 260 in dc30b92 pub fn current() -> CiEnv { 
- change this to give a hard error locally if git fails
I have a slight preference for the latter but I'm ok with either.
02ff160    to
    f87e7af      
    Compare
  
    d19587b    to
    83bab41      
    Compare
  
    83bab41    to
    3276879      
    Compare
  
    f341a3e    to
    0815bc9      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
0815bc9    to
    c6d6073      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
| @bors r+ Thanks! | 
…-only-bors-is-allowed-to-do-that, r=jyn514 Add tidy check to deny merge commits This will prevent users with the pre-push hook from pushing a merge commit. Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future. I added a link to `@jyn514's` blog post for the error case because that's the best resource to solve merge commits. But it would probably be better if it was integrated into https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, then we could link that instead. r? `@jyn514`
| ☀️ Test successful - checks-actions | 
Revert "Auto merge of rust-lang#105058 - Nilstrieb:no-merge-commits, r=jyn514" This reverts commit 4839886, reversing changes made to ce85c98. Fixes rust-lang#106232 (comment). r? `@jyn514`
| Finished benchmarking commit (4839886): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. 
 CyclesThis benchmark run did not return any relevant results for this metric. | 
| But this don't work? #99339 (comment) | 
Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?
Rollup merge of rust-lang#114157 - pitaj:triagebot_no-merges, r=ehuss Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?
This will prevent users with the pre-push hook from pushing a merge commit.
Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future.
I added a link to @jyn514's blog post for the error case because that's the best resource to solve merge commits. But it would probably be better if it was integrated into https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, then we could link that instead.
r? @jyn514